-
Notifications
You must be signed in to change notification settings - Fork 13.9k
a collection of simple const changes #146600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This consists mostly of derive changes or simple impl changes.
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing anything const in this module is a bit confusing as we can't actually make anything interesting const. I'd rather not change anything in the fmt machinery that isn't clearly connected to a use case
)+) => {$( | ||
#[derive(Clone, Copy, Eq)] | ||
#[derive(Copy)] | ||
#[derive_const(Clone, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that after the pattern types refactor lands. Otherwise I may just have to temporarily remove it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironically, this is the only change I actually need to constify all the slice and array types and their dependents. Are you sure this is blocked? I'll file a separate PR with that enablement path made obvious and request you as reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I may have resolved this problem.
#[derive_const(Clone, PartialEq, Eq, PartialOrd, Ord)] | ||
#[lang = "coroutine_state"] | ||
#[unstable(feature = "coroutine_trait", issue = "43122")] | ||
pub enum CoroutineState<Y, R> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coroutines have no support for getting polled in const code. Let's pair this constification with actual coroutine constifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many other types that don't really make sense in const contexts yet that you constified impls on. I'm not sure how useful this is. I guess we do need PRs like this at some point just to finish constification. But rn it just feels like a hard to review PR that is easy to write
That's a fair critique. I was honestly trying to fix some of the core paths and realized that there was a lot that could be const that nobody had done yet. So I was just trying to be helpful. |
No description provided.